[rocprofiler-sdk] Enable region argument annotation in perfetto output trace#3854
[rocprofiler-sdk] Enable region argument annotation in perfetto output trace#3854
Conversation
fd77ad6 to
bb0e3fe
Compare
bb0e3fe to
88e23bf
Compare
| rocprofiler_add_integration_execute_test( | ||
| rocprofv3-test-rocpd-perfetto-generation-annotations | ||
| COMMAND | ||
| ${Python3_EXECUTABLE} -m rocpd convert -f pftrace --kernel-rename --annotate-args | ||
| -d ${CMAKE_CURRENT_BINARY_DIR}/rocpd-output-data-annotations -i | ||
| ${CMAKE_CURRENT_BINARY_DIR}/rocpd-input-data/out_results.db | ||
| DEPENDS rocprofiler-sdk::rocprofv3 | ||
| TIMEOUT 120 | ||
| LABELS "integration-tests;rocpd" | ||
| PRELOAD "${ROCPROFILER_MEMCHECK_PRELOAD_ENV_VALUE}" | ||
| ENVIRONMENT "${rocprofv3-rocpd-env}" | ||
| FIXTURES_SETUP rocprofv3-test-rocpd-generation-annotations | ||
| FIXTURES_REQUIRED rocprofv3-test-rocpd) | ||
|
|
There was a problem hiding this comment.
This looks like just a duplication of the prior execution_test, with the added --annotate-args param. Do you want to just add --annotate-args to the original test, and call your validate_annotations.py using that file?
There was a problem hiding this comment.
I was thinking of keeping this separate because --annotate-args is optional mode rather than the default path. Also, if we add more options for rocpd convert and more tests in future, it may be cleaner to keep separate execute tests so it’s easier to debug issues. But I do see your point about avoiding duplications. I’m fine with whichever direction we want to standardize on.
There was a problem hiding this comment.
OK, my thought is that your validation is going to check the annotations later anyway. So we can save 1 test & time by just lumping the 2 generation tests into 1. Just thought I'd call it out, I'm ok either way on this one too.
There was a problem hiding this comment.
Yeah, we should minimize the generation tests.
There was a problem hiding this comment.
Wait disregard the previous comment. I thought this was running an app on the GPU
projects/rocprofiler-sdk/tests/pytest-packages/tests/rocprofv3.py
Outdated
Show resolved
Hide resolved
projects/rocprofiler-sdk/source/lib/python/rocpd/source/perfetto.cpp
Outdated
Show resolved
Hide resolved
88e23bf to
239c4c9
Compare
yhuiYH
left a comment
There was a problem hiding this comment.
LGTM. CI tests are queued because the pool has been re-purposed. But it was passing before and I checked the tests locally, seems to be passing.
jrmadsen
left a comment
There was a problem hiding this comment.
Why not just annotate all the rocpd_args associated with the event_id? Wouldn't this include the event handle?
4a2cee2 to
8137764
Compare
@jrmadsen sure, please see enable arg annotation for all instead of only event ID. Updated tests as well to validate |
8137764 to
1fe0a46
Compare
1fe0a46 to
ca1ea6e
Compare
| _perform_csv_json_match(a, b, keys_mapping[category], json_data) | ||
|
|
||
|
|
||
| def test_perfetto_arg_annotations(pftrace_data, pftrace_filename): |
There was a problem hiding this comment.
Isn't pftrace_data the PerfettoReader of pftrace_filename?
There was a problem hiding this comment.
| rocprofiler_add_integration_execute_test( | ||
| rocprofv3-test-rocpd-perfetto-generation-annotations | ||
| COMMAND | ||
| ${Python3_EXECUTABLE} -m rocpd convert -f pftrace --kernel-rename --annotate-args | ||
| -d ${CMAKE_CURRENT_BINARY_DIR}/rocpd-output-data-annotations -i | ||
| ${CMAKE_CURRENT_BINARY_DIR}/rocpd-input-data/out_results.db | ||
| DEPENDS rocprofiler-sdk::rocprofv3 | ||
| TIMEOUT 120 | ||
| LABELS "integration-tests;rocpd" | ||
| PRELOAD "${ROCPROFILER_MEMCHECK_PRELOAD_ENV_VALUE}" | ||
| ENVIRONMENT "${rocprofv3-rocpd-env}" | ||
| FIXTURES_SETUP rocprofv3-test-rocpd-generation-annotations | ||
| FIXTURES_REQUIRED rocprofv3-test-rocpd) | ||
|
|
There was a problem hiding this comment.
Yeah, we should minimize the generation tests.
| import pytest | ||
|
|
||
|
|
||
| def test_arg_annotations(pftrace_data, request): |
There was a problem hiding this comment.
| def test_arg_annotations(pftrace_data, request): | |
| def test_arg_annotations(pftrace_reader): |
Just make a fixture that drops .read()[0] from pftrace_data so that we aren't creating two readers of the same file.
There was a problem hiding this comment.
Sure, done in cleanup: remove unnecessary PerfettoReader call
Motivation
This PR enables function argument annotation in Perfetto with
--annotate-args.Without info like event ID arg value, it may be hard to follow event dependencies and debug HIP event synchronization.
The goal is disambiguating waits across multiple events/streams: when a stream waits on different events originating from different streams, having the event ID allows them to correlate
hipStreamWaitEventwith the correspondinghipEventRecordcalls and identify which event (and thus which upstream stream/work) is actually blocking execution.With this PR: When
--annotate-args(introduced in #340) is used in rocpd to perfetto conversion, the function arguments get annotatedTechnical Details
--annotate-argsJIRA ID
Resolves AIPROFSDK-195
Test Plan
Local testing:
Test Result
Submission Checklist